Allow disabling step sourcemap with new sourcemap option in builders#1842
Allow disabling step sourcemap with new sourcemap option in builders#1842VaguelySerious merged 8 commits intomainfrom
sourcemap option in builders#1842Conversation
Signed-off-by: Peter Wielander <mittgfu@gmail.com>
🦋 Changeset detectedLatest commit: 7065f19 The changes in this PR will be included in the next version bump. This PR includes changesets to release 18 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
🧪 E2E Test Results✅ All tests passed Summary
Details by Category✅ ▲ Vercel Production
✅ 💻 Local Development
✅ 📦 Local Production
✅ 🐘 Local Postgres
✅ 🪟 Windows
✅ 📋 Other
|
📊 Benchmark Results
workflow with no steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Express | Nitro | Next.js (Turbopack) workflow with 1 step💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Express | Next.js (Turbopack) workflow with 10 sequential steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Express | Next.js (Turbopack) workflow with 25 sequential steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Express | Next.js (Turbopack) workflow with 50 sequential steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Express | Next.js (Turbopack) | Nitro Promise.all with 10 concurrent steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Next.js (Turbopack) | Express Promise.all with 25 concurrent steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Express | Next.js (Turbopack) Promise.all with 50 concurrent steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Express | Next.js (Turbopack) Promise.race with 10 concurrent steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Express | Next.js (Turbopack) Promise.race with 25 concurrent steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Express | Nitro | Next.js (Turbopack) Promise.race with 50 concurrent steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Express | Next.js (Turbopack) | Nitro workflow with 10 sequential data payload steps (10KB)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Express | Nitro | Next.js (Turbopack) workflow with 25 sequential data payload steps (10KB)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Express | Next.js (Turbopack) workflow with 50 sequential data payload steps (10KB)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Express | Nitro | Next.js (Turbopack) workflow with 10 concurrent data payload steps (10KB)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Express | Nitro | Next.js (Turbopack) workflow with 25 concurrent data payload steps (10KB)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Express | Next.js (Turbopack) | Nitro workflow with 50 concurrent data payload steps (10KB)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Express | Next.js (Turbopack) Stream Benchmarks (includes TTFB metrics)workflow with stream💻 Local Development
▲ Production (Vercel)
🔍 Observability: Express | Nitro | Next.js (Turbopack) stream pipeline with 5 transform steps (1MB)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Express | Nitro | Next.js (Turbopack) 10 parallel streams (1MB each)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Express | Next.js (Turbopack) fan-out fan-in 10 streams (1MB each)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Express | Next.js (Turbopack) SummaryFastest Framework by WorldWinner determined by most benchmark wins
Fastest World by FrameworkWinner determined by most benchmark wins
Column Definitions
Worlds:
Check the workflow run for details. |
|
(Coming over from #1843, which I just closed as a duplicate — this PR is more comprehensive, particularly the framework-wide coverage and docs updates.) A few suggestions that came out of that branch, in case they're useful: 1. Consider using esbuild's native sourcemap vocabularyInstead of
Concretely the type would be: export type SourcemapMode = boolean | 'inline' | 'linked' | 'external' | 'both';2. Add a
|
Replace the bespoke `boolean | 'inline' | 'disabled'` type with esbuild's full `SourcemapMode` vocabulary (adds `'linked'`, `'external'`, `'both'`) so users can ship sourcemaps to observability tooling without bloating the function bundle. Add a `WORKFLOW_SOURCEMAP` env var as a cross-framework override; config wins over env, env wins over the per-bundle default. Flip `shouldAddSourcemapSupport` to track `sourcemapsEnabled` on the Vercel step function so disabling sourcemaps also drops the runtime shim. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Signed-off-by: Peter Wielander <mittgfu@gmail.com>
TooTallNate
left a comment
There was a problem hiding this comment.
Verified the implementation end-to-end. The shape is right and the precedence rules are well-tested.
What's right
- Type vocabulary matches esbuild (
boolean | 'inline' | 'linked' | 'external' | 'both'). Users can pass through whatever esbuild accepts without the SDK getting in the way. - Precedence is correct and tested: explicit
config.sourcemap>WORKFLOW_SOURCEMAPenv > per-call-site default. The 10 new tests inresolve-sourcemap.test.tscover every branch including the env-var aliases (0/1/true/false), unrecognized values (warn + fall through), and thesourcemapsEnabledderived getter. All 145 builders tests pass on the branch. sourcemapsEnabledis the right primitive for downstream decisions like skipping the source-map-support runtime shim (vercel-build-output-api.ts:86). That shim isn't trivial in size, so dropping it forsourcemap: falseis a meaningful chunk of the 250MB savings.- Backwards compat preserved: the legacy
WORKFLOW_EMIT_SOURCEMAPS_FOR_DEBUGGING=1keeps working as the per-call-site default for the wrapper/webhook bundles. The "minor semantics change" the changeset flags (explicitsourcemap: ...now overrides the legacy env for those bundles too) is the right resolution — it's the natural meaning of "explicit > env > default" — and the changeset is upfront about it. - Discover-phase
sourcemap: falseat line 295 is left untouched. That's correct —write: falsemeans the discover pass doesn't emit anything, sourcemaps would be wasted compute. - Plumbing is consistent across all five framework adapters (Astro, NestJS, Next.js, Nitro, SvelteKit) — same option name, same type, same docs blurb pattern. Nuxt flows through to Nitro transitively (no separate plumbing needed since Nuxt is already a thin wrapper), so its absence from the changeset is correct.
- CLI works via env var route —
WORKFLOW_SOURCEMAP=false workflow buildworks without any CLI plumbing because all builders inheritBaseBuilder.resolveSourcemap. So the CLI/express/fastify/hono getting-started paths can use this without an additional config layer. - Documentation is comprehensive: every framework getting-started page that shows config gets a sourcemap row, plus a dedicated "Source maps" section in
with-workflow.mdxcovering all the modes, the size-vs-stack-trace tradeoff, and the env var precedence.
Nits (non-blocking)
- Type duplication.
SourcemapModeis exported from@workflow/builders/types.tsbut then redefined inline in 6 places (the framework adapter types and a couple of doc strings). All five framework packages already depend on@workflow/builders, so they could justimport type { SourcemapMode }and reuse. If the union ever grows (e.g. esbuild adds a new mode), all 6 sites have to be updated in lockstep. Trivial cleanup, would also DRY up the doc comments which are nearly identical. @workflow/nuxtgetting-started docs don't mentionsourcemapeven though it flows through to Nitro and works. Thenuxt.mdxpage doesn't have a config-options table at all currently, so this isn't a regression — but consider adding parallel coverage for completeness.parseSourcemapEnvwarning writes directly toconsole.warn. Most of the codebase uses the structuredruntimeLoggerfor this kind of thing. Probably fine for build-time warnings but worth aligning if there's a builder-side logger.
Tested
pnpm vitest run packages/builders/src/resolve-sourcemap.test.ts→ 10/10 pass.pnpm testinpackages/builders→ 145/145 pass.
LGTM.
No description provided.